-
Notifications
You must be signed in to change notification settings - Fork 934
fix(load): add support for factory-callback parser presets #834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
config.parserPreset = { | ||
name: config.parserPreset, | ||
path: resolvedParserPreset, | ||
parserOpts: resolvedParserConfig.parserOpts | ||
parserOpts: require(resolvedParserPreset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code before the first attempt was (await require(resolvedParserPreset)).parserOpts
, which is not going to work with the newer callback-presets. I took a look at @commitlint/resolve-extended
and it does exactly the same as written here, I think that's a better way-to-go compared to the instant-extraction of parserOpts
(like the code I just mentioned).
typeof parser.parserOpts === 'object' && | ||
typeof parser.parserOpts.parserOpts === 'object' | ||
) { | ||
return parser.parserOpts.parserOpts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this "happens", but I think it's related to the context-overwrite when calling @commitlint/resolve-extended
. The failing test was the parser-preset-override
, I don't think anything is wrong with that tests though.
What I think what happens:
commitlint.config.js
overwritesparserPreset
with a module- overwrite is picked up and module is loaded
- due to this change, it's not using
parserOpts
but the parser preset itself.
'fixtures/recursive-parser-preset-conventional-atom' | ||
); | ||
// the package file is nested in 2 folders, `npm.bootstrap` cant do that | ||
await execa('npm', ['install'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is any other way, let me know 😄 I think this is not that bad, but I didn't see any other option for now.
d32a641
to
c705cd4
Compare
Description
Fixes #830
Motivation and Context
Not all presets worked flawlessly with the existing load-preset code. The code had no support for presets that required a initialise-callback. I'm not sure if the "example atom preset" from here worked because of this. But this PR helps solving that issue.
We could also integrate the
conventional-changelog-loader
into@commitlint/load
, didn't have much luck with that but I could try implementing it again. I do have some doubt if it will remove some of the code already added in this PR, because we still have to initialise it.Usage examplesHow Has This Been Tested?
Instead of creating similar-structured presets, I added the actual preset modules in the tests. So far, I could find 3 different types of presets.
{ parserOpts: {} }
, like angular(null, { parserOpts: {} })
, like atomparserOpts
as function), like conventionalcommitsAll these changelogs are now feature-tested and included in the PR.
Types of changes
Checklist: